Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add Discard Handling for AuthenticationError #2239

Merged
merged 7 commits into from
Jul 5, 2024

Conversation

brunomiguelpinto
Copy link
Contributor

@brunomiguelpinto brunomiguelpinto commented Jul 2, 2024

Context:

In this pull request, we change the error handling within the StripeCreateJob by discarding jobs that encounter Stripe::AuthenticationError. This change ensures that we prevent creating noise

Description:

  • When a Stripe::AuthenticationError is raised, it indicates an issue with the authentication credentials. Since this is a non-recoverable error in the job context, retries are unnecessary.
  • Customers are already notified of such errors through a webhook, making additional retries redundant and potentially confusing for the customer.

Additional Context:

  • The update_payment_method_id method in the related service sends a webhook notification when any Stripe::StripeError is raised, ensuring customers are informed of issues promptly.
Screenshot 2024-07-02 at 18 30 10

@brunomiguelpinto brunomiguelpinto changed the title feat: discard AuthenticationError feat: Add Discard Handling for AuthenticationError Jul 2, 2024
@brunomiguelpinto brunomiguelpinto self-assigned this Jul 2, 2024
@brunomiguelpinto brunomiguelpinto marked this pull request as ready for review July 2, 2024 17:38
Copy link
Contributor

@julienbourdeau julienbourdeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL discard_on

I always rescued and ignore in perform. I like discard_on much better.

@vincent-pochet
Copy link
Collaborator

Some other errors are handled here:
https://github.com/getlago/lago-api/blob/main/app/services/invoices/payments/stripe_service.rb#L193

Not sure but I think we should do the same things here rather than just silencing the error.

  • Optionally (since it seems already OK): Notifying the owner of the account that its stripe account is not correctly configured
  • Setting the payment status to failed

brunomiguelpinto and others added 2 commits July 5, 2024 08:58
Co-authored-by: Vincent Pochet <vincent@getlago.com>
@brunomiguelpinto brunomiguelpinto merged commit 1ef986c into main Jul 5, 2024
6 checks passed
@brunomiguelpinto brunomiguelpinto deleted the feat-handle-stripe-authentication-error branch July 5, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants